fix(rust/core): Handle the ASCII characters from sqlstate instead of their decimal values#4141
fix(rust/core): Handle the ASCII characters from sqlstate instead of their decimal values#4141felipecrv wants to merge 1 commit into
Conversation
…their decimal values
|
I'm tired of seeing |
| } else if c >= 32 && c <= 126 { | ||
| char::from(c as u8) | ||
| } else { | ||
| '\u{FFFD}' |
There was a problem hiding this comment.
Maybe we can still display the value somehow instead of hiding it? (Just stringify the integer value instead?)
There was a problem hiding this comment.
I'm not hiding. I'm showing � to say "this is not valid UTF-8". SQLSTATE is ASCII/UTF-8. A C implementation of this wouldn't even bother validating the value. Would just be printing it from a char *.
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| let safe_ascii = |c: c_char| -> char { | ||
| if c == 0 { | ||
| '0' |
There was a problem hiding this comment.
I think I'd prefer something that isn't ambiguous with an actual sqlstate of '00000'.
There was a problem hiding this comment.
But this is because drivers can either set the SQLSTATE to all 0 integers or '0' ASCII. They are all equivalent to the end-user: they mean "empty SQLSTATE".
There was a problem hiding this comment.
At least as a developer, though, I'd like to know the exact values. I agree that we should handle the common-case, but I don't think we have to sacrifice accuracy
There was a problem hiding this comment.
That's why Rust (wisely) has the fmt::Debug and fmt::Display traits. Debug is for the developer. The derived Debug will print exactly what you are asking. Display is the "human" rendering.
When I do a to_string() on a error, Display::fmt is called because I'm rending a message to a human.
| f, | ||
| "{:?}: {} (sqlstate: {:?}, vendor_code: {})", | ||
| self.status, self.message, self.sqlstate, self.vendor_code | ||
| "{:?}: {} (sqlstate: {}{}{}{}{}, vendor_code: {})", |
There was a problem hiding this comment.
Maybe what'd be better is: if sqlstate is all-NUL, omit it; if all-ASCII, stringify it; else fall back to the current representation?
There was a problem hiding this comment.
Same as the comment above:
But this is because drivers can either set the SQLSTATE to all 0 integers or '0' ASCII. They are all equivalent to the end-user: they mean "empty SQLSTATE".
|
@felipecrv sorry for losing track of this. Do you want to rebase and fix the Clippy lint and then we can get this merged? I'll give in to your reasoning here ^_^ |
2 participants
No description provided.